feat: add require.ensure error callback
#48
Conversation
|
|
michael-ciniawsky
requested review from
bebraw,
joshwiens and
michael-ciniawsky
|
Agreed about the tests. Too bad there weren't any tests to begin with... The require.ensure with error handling does have tests though, so I think it would be sufficient to just test that the correct syntax of require.ensure is called by bundle-loader (for the various permutations of with/without error callback, and with/without chunk name.) Are there any webpack loaders with tests like that that I could use as a reference for adding some tests to bundle-loader? Also, what is |
It's a default infrastructure we use for loaders and packages. It comes with opinionated testing setup etc. and allows us to maintain the setup as a dependency (it syncs and migrates project config). |
michael-ciniawsky
added
semver: Major
type: Feature
and removed
pr_action: Discuss
labels
|
@d3viant0ne Ok, I'm still a little unsure as to how to proceed with creating tests for this package, if that's a hard requirement for getting my PR merged... If it helps put minds at east, I have been using this code in production for months now with no issues via a fork of bundle-loader which is identical to this PR: https://github.com/jharris4/split-chunk-loader |
michael-ciniawsky
changed the title
|
First and foremost, the pull request for defaults has to land in master which will lay the foundations to actually author tests and landing this with defaults is more about not releasing back to back Major version releases than anything else. As far as tests for this specific pull request, we need to add tests to cover the existing functionality. If those don't provide a path forward, we can deal with it then. |
|
Any word on the defaults? Anything I can do to help move this forward and get it merged? |
|
@jharris4 Should be on the finishing line, sry for the inconvenience |
|
Literally working on it now :) |
|
Hope the next release will coming soon! Currently I just edit line.27 to handle loading errors. " }, function(err) { cb(null, err); }, " + chunkNameParam + ");\n", |
|
@d3viant0ne Any updates on the status of this? Anything I can do to try and help speed things along? |
|
@jharris4 - It's technically done. I'll pick up the latest version of defaults & get the other org maintainers to give it a final once over before it merges. |
|
@d3viant0ne Still curious to see if this is gonna move forward :-) If it helps to get things moving I can try and do some work to integrate |
index.js
Outdated
| "module.exports = function(cb) {\n", | ||
| " require.ensure([], function(require) {\n", | ||
| " cb(require(", loaderUtils.stringifyRequest(this, "!!" + remainingRequest), "));\n", | ||
| "module.exports = function(successCallback, errorCallback) {\n", |
There was a problem hiding this comment.
function (successCallback, errorCallback) => function(cb, err)
There was a problem hiding this comment.
I think you were suggesting rename successCallback to cb and errorCallback to err so I've done that.
index.js
Outdated
| " cbs = null;\n", | ||
| " for(var i = 0, l = callbacks.length; i < l; i++) {\n", | ||
| " callbacks[i](data);\n", | ||
| "var cbs,\n", |
There was a problem hiding this comment.
Please remove all ending \n in the code snippets and instead result.join('\n') them on line 73
There was a problem hiding this comment.
Good catch! I've made the change.
michael-ciniawsky
changed the title
require.ensure error callback
|
@michael-ciniawsky I made the changes you requested. Dare I hope that you guys might be close to actually merging this? :-) |
|
Any intent of merging this? |
|
@michael-ciniawsky @d3viant0ne Hi guys, so it has been 11 months since I created this PR! Is there anything I can do to move this forward? I'm starting to look at upgrading to webpack 4 and hopefully this loader won't require any changes for that... |
|
@michael-ciniawsky @d3viant0ne I just merged the changes from version 0.5.6 so the build should be passing again soon. |
|
@michael-ciniawsky @d3viant0ne Or not! No idea why the travis builds are failing... Something about the |
|
@michael-ciniawsky @d3viant0ne I just fixed a small issue with the error callback not getting passed the error object in non-lazy mode. We've been using this in production for months now. Any chance we can get this merged so that I can stop maintaining https://github.com/jharris4/split-chunk-loader ? :-) |
|
I'm using the code from this PR and it's working well. Is there any hope with getting this merged? |
Adds support for an optional error callback to the bundle-loader.
This work was originally done by @richardscarrott: https://github.com/richardscarrott/require-error-handler-webpack-plugin/blob/master/src/BundleLoader.js.
This PR uses the version created by @richardscarrott with refactoring to reflect changes from the bundle-loader repo since the fork was created.